-
-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GSoC 2024: Internationalization Implementation for Zimit Frontend #51
Conversation
@Onyx2406 Thank you for your PR, here a few questions:
|
It does fix #46, but this PR only adds translations to the strings within this repository using Vue I18n. It doesn't translate the form details from Zimfarm. This PR only contains code to solve #46 and not #28. (I wanted to refer to openzim/overview#28). |
@Nikerabbit @Onyx2406 Do you know if this translation file can be indgested by Translatewiki? |
@kelson42, they are using a similar structure at https://github.com/mathjax/MathJax-i18n, so it should work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you; it's promising.
- We need RTL support
- I see that
human_size_limit
andhuman_time_limit
are not i18n - Languages list if static at the moment: requiring manually modifying two files to add a new locale. With translatewiki adding more all the time, we'd want something automatic
FYI (but it's a different PR) I believe there might be some strings in the backend (error message?) that would also require integration to completely close this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good job, I'm happy to see we are already quite close to have completed this task.
I think you've forgot to i18n the footer as well.
I propose to split the integration with TranslateWiki to another PR, we would already benefit from merging this PR once concerns have been addressed (Right To Left support is very important, I don't know if it works as of today, please test and confirm/adapt). @rgaudin are you ok with it?
Regarding @rgaudin comment about the automatic detection of new languages added, I do not think it is mandatory, especially it should not break lazy loading of translations. I do not mind to have to edit one more file everytime a new language is added (I imagine TranslateWiki folks might even be able to do it themselves when they add new languages).
Would you mind to create the "secret" qqq
language where TranslateWiki expects to have translation instructions in English?
TranslateWiki Integration: Formal integration with TranslateWiki is pending.
I will take care of that once PR is merged
Automated Browser Language Detection Improvement: While initial support for detecting the browser's language is implemented, further testing and refinements is needed across different browsers and user configurations.
Do you have something special in mind to be tested? Or is it just "normal" testing?
Preparation for TranslateWiki: Organizing and documenting the translation file structure to facilitate future integration with TranslateWiki.
I think the only thing left is to add the qqq
magic language with translation instructions. Do you have something else in mind?
FYI (but it's a different PR) I believe there might be some strings in the backend (error message?) that would also require integration to completely close this
Definitely a second step, let's finish this first one for now.
Yes, we do support nested JSON. Also FYI we have recently updated our guidance for new projects: https://translatewiki.net/wiki/Translating:New_project |
Added qqq.json file containing translation instructions and context in English.
@benoit74 Regarding splitting the integration with TranslateWiki into another PR, I completely agree. I have added the footer i18n. I'll ensure that RTL support is tested and functional before merging. However, as the application currently supports only two languages which don't require RTL support, We can add RTL support to a future PR when additional RTL languages are added? I've added the qqq language file for translation instructions. For the automated browser language detection improvement, I'll do further testing across different browsers, so it's a normal testing and nothing special. |
Thank you ! Regarding RTL support, I do not mind to split it into another PR if there is too much work. It is however a pre-requisite for TranslateWiki integration and mostly a requirement for us, we have to translate the website into Farsi / Persian and already have persons ready to do the translation, so it will be in use quite quickly after TranslateWiki integration. Doesn't need to be perfect, but at least it should look pretty (e.g. I don't think we necessarily have to transfer all left buttons to the right and vice-versa, but at least the text should be readable and preferably right aligned when it was left aligned). |
Alright. In that case, it's better to push commits for RTL Support in this PR itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, we are getting closer but you broke few things in your last commits (see conversations)
Please do not spend too much time fine-tuning the exact words of French and Farsi translations, since you've made them with an automated translation tool we will not merge them anyway, they will have to be translated by native speakers on TranslateWiki.
Other topics to address:
- you updated all locales but not the
qqq.json
, while it is probably the most important one - why did you sometimes place HTML code inside the text to translate (e.g.
app.thanksToMozilla
) and other times you split the text is many substrings (e.gfaq.gotErrorDesc1
andfaq.gotErrorDesc2
) ? I believe it would be way simpler in most situations (e.g. not when HTML code is too complex, but in every other situations) to always have HTML code inside the translation and one single string to translate, it will make the translator work way easier rather than being presented with only few words out of any context, or even a dot or a comma to not forget at the beginning of a translation- e.g you should have only one
footer
string (app
is a bit vague) and its value should bePowered by <a target="_blank" href="https://kiwix.org">Kiwix</a> and <a target="_blank" href="https://webrecorder.net">Webrecorder</a>, thanks to a <a target="_blank" href="https://www.mozilla.org/moss/">Mozilla Open-Source Support</a> Award
- e.g you should have only one
- limits in Request.vue JS code are still not handled properly:
- they are not translated like in the Faq.vue
- pluralization is not handled
- "+" buttons of the FAQ are still not on the left when RTL language is used
- please have a look, if it is too complex I do not mind to move this to an issue for later fix, it is not a big usability concern
… within translation strings for better clarity and ease of translation. Properly handled limits and implemented pluralization in Request.vue. Fixed missing localized strings in Request.vue and NewRequest.vue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, we are getting closer to merging this.
Still some comments in conversation plus few below.
- I just realized that the zimit logo was previously centered on the page while it is not left or right ; you must put it back to center (sorry to not have realized this before, I did not got it right)
- FYI, we will not integrate with TranslateWiki right away, I will open an issue with details on what still has to be done (but does not have to be addressed in this issue)
New issue about what is left (not part of this issue / PR): #53 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not working as expected, logo size is altered.
Wouldn't it be simpler to move the select box out of the header, keep the header as it was and put an absolute positioning to the select box?
This worked, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing ... Are you constantly generating code with a code generator? This is pretty annoying to spend so much time in reviews detecting code which means just nothing, or changes which are meaningless (random spaces added everywhere, ...)
Thank you, unfortunately you again modified the indentation of many files. I'm a bit sorry about that, but I will take over the end of this PR from this point, because I need to activate prettier and eslint on this, and this is way beyond the scope of this PR. Please consider that work is done from this point, I will finish what needs to be done. I will convert this to draft so that it is not merged by mistake. Thank you. |
Done on our own finally |
Fixes #46
See also openzim/overview#28
Overview
This pull request introduces comprehensive internationalization (i18n) support to the Zimit frontend.
What Has Been Done
What Is Left
Screenshots